Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Use curl to download sample data instead of wget. #13761

Merged
merged 1 commit into from
Jan 8, 2019
Merged

Use curl to download sample data instead of wget. #13761

merged 1 commit into from
Jan 8, 2019

Conversation

frankfliu
Copy link
Contributor

Description

On Mac OSX, wget is conflicting with MXNet build (zlib), developer has to uninstall wget from Mac.
Current scala example script using wget to download model and images, make it difficult to mac developers.

This change to workaround this issue form Mac users.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@frankfliu
Copy link
Contributor Author

@mxnet-label-bot [scala]

@Roshrini
Copy link
Member

Roshrini commented Jan 2, 2019

@mxnet-label-bot Add [Scala, pr-awaiting-review]
@lanking520 @zachgk For review

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review Scala labels Jan 2, 2019
lanking520
lanking520 reviewed Jan 2, 2019
View reviewed changes
scala-package/examples/scripts/rnn/run_test_charrnn.sh
@@ -27,7 +27,7 @@ else
fi

# you can get the training data file using the following command
# wget http://data.mxnet.io/data/char_lstm.zip
# curl -O http://data.mxnet.io/data/char_lstm.zip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-o?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-o need a path as parameter,
-O will use file name in url and save in current directory.

scala-package/examples/scripts/rnn/run_train_charrnn.sh
@@ -28,7 +28,7 @@ fi
# which gpu card to use, -1 means cpu
GPU=$1
# you can get the training data file using the following command
# wget http://data.mxnet.io/data/char_lstm.zip
# curl -O http://data.mxnet.io/data/char_lstm.zip
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-o?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downloads with the same name according to the manpage. (man curl)

lanking520
lanking520 approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@lanking520 lanking520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your contribution

piyushghai
piyushghai approved these changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@piyushghai piyushghai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me!
Thanks for making this change :)

@vishaalkapoor
Copy link
Contributor

lgtm.

@lanking520 lanking520 merged commit 7b7fd15 into apache:master Jan 8, 2019
@frankfliu frankfliu deleted the wget branch January 8, 2019 16:52
rondogency pushed a commit to rondogency/incubator-mxnet that referenced this pull request Jan 9, 2019
@frankfliu
Use curl to download sample data instead of wget. (apache#13761)
a0859b5
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@frankfliu
Use curl to download sample data instead of wget. (apache#13761)
0b3be69
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@larroy larroy larroy left review comments

@lanking520 lanking520 lanking520 approved these changes

@piyushghai piyushghai piyushghai approved these changes

@nswamy nswamy Awaiting requested review from nswamy nswamy is a code owner

@yzhliu yzhliu Awaiting requested review from yzhliu yzhliu is a code owner

Assignees
No one assigned
Labels
pr-awaiting-review PR is waiting for code review Scala
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@frankfliu @Roshrini @vishaalkapoor @larroy @piyushghai @lanking520 @marcoabreu